Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libclc] Separate out generic AS support macros #13792

Merged
merged 3 commits into from
May 21, 2024

Conversation

frasercrmck
Copy link
Contributor

The previous model where we declare/define builtins based on whether target supports the generic address space was not able to capture the full complexity of what we need.

We could see this in the fact that the two targets which we marked as not supporting the generic AS do in fact support it. The problem is rather that the target AS they map the generic AS to is the same target AS mapped to by the private AS.

This problem hasn't properly been made apparent because all builtins we want to support with the generic AS also have overloads on the private AS. However, as can be seen in PRs like #13428, some targets want to support generic AS overloads of atomic builtins which don't also have private AS overloads. It's here that the simple dichotomy breaks down.

This patch splits up the support of the generic AS into:

  1. The target doesn't support the generic AS
  2. The target supports the generic AS as a distinct target AS
  3. The target supports the generic AS mapped identically as the private the AS

All of our previous uses of case 1 have been migrated to case 3. These targets can make use of case 2 in the future to support generic AS builtins where no private AS builtins are supported.

Note how we hardcode the assumption that the clash is on the private address space. This is unfortunate but for simplicity as it's the case for the two targets we care about. It was already being made, but in a less obvious way. There are ways of loosening this assumption if we ever needed to but it would currently complicate the code for untested scenarios.

The previous model where we declare/define builtins based on whether
target supports the generic address space was not able to capture the
full complexity of what we need.

We could see this in the fact that the two targets which we marked as
not supporting the generic AS do in fact support it. The problem is
rather that the target AS they map the generic AS to is the same target
AS mapped to by the private AS.

This problem hasn't properly been made apparent because all builtins we
want to support with the generic AS also have overloads on the private
AS. However, as can be seen in PRs like intel#13428, some targets want to
support generic AS overloads of atomic builtins which don't also have
private AS overloads. It's here that the simple dichotomy breaks down.

This patch splits up the support of the generic AS into:

1. The target doesn't support the generic AS
2. The target supports the generic AS as a distinct target AS
3. The target supports the generic AS mapped identically as the private
   the AS

All of our previous uses of case 1 have been migrated to case 3. These
targets can make use of case 2 in the future to support generic AS
builtins where no private AS builtins are supported.

Note how we hardcode the assumption that the clash is on the private
address space. This is unfortunate but for simplicity as it's the case
for the two targets we care about. It was already being made, but in a
less obvious way. There are ways of loosening this assumption if we ever
needed to but it would currently complicate the code for untested
scenarios.
@frasercrmck frasercrmck requested a review from a team as a code owner May 15, 2024 14:54
@frasercrmck frasercrmck requested a review from steffenlarsen May 15, 2024 14:54
Copy link
Contributor

@MartinWehking MartinWehking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@frasercrmck
Copy link
Contributor Author

@intel/llvm-gatekeepers this looks good to merge, thanks

@martygrant martygrant merged commit bdaf1e2 into intel:sycl May 21, 2024
14 checks passed
@frasercrmck frasercrmck deleted the libclc-separate-generic-as-macros branch May 21, 2024 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants